-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add redirect util function for project custom actions #1865
Add redirect util function for project custom actions #1865
Conversation
WalkthroughThe changes in this pull request involve updates to the configuration of environment variables and the introduction of a new utility function for handling redirect URLs in AdminJS actions. Specifically, the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant AdminJSUtils
Client->>Server: Make AdminJS request
Server->>AdminJSUtils: Call getRedirectUrl(request, resourceName)
AdminJSUtils->>Server: Return redirect URL
Server->>Client: Respond with redirect URL
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/server/adminJs/adminJsUtils.ts (2)
1-6
: Enhance function documentation for better maintainability.The JSDoc comments could be improved with:
- More specific type information for the request parameter
- Documentation for potential errors
- Example usage
Consider updating the documentation:
/** * Extracts the redirect URL from request headers for AdminJS actions - * @param request - The AdminJS action request object + * @param request - The AdminJS action request object containing rawHeaders * @param resourceName - The name of the resource (e.g., 'Project', 'User') * @returns The URL to redirect to after action completion + * @throws {Error} When URL parsing fails (caught internally) + * @example + * // Returns '/admin/resources/Project?page=1' + * getRedirectUrl(request, 'Project') */
19-21
: Enhance error logging for better debugging.The current error logging could be more informative by including the referrer URL that caused the error.
-console.warn('Error parsing referrer URL:', error); +console.warn('Error parsing referrer URL:', { referrerUrl, error: error.message });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/config.ts (0 hunks)
- src/server/adminJs/adminJsUtils.ts (1 hunks)
- src/server/adminJs/tabs/projectsTab.ts (9 hunks)
💤 Files with no reviewable changes (1)
- src/config.ts
🔇 Additional comments (5)
src/server/adminJs/adminJsUtils.ts (1)
14-24
: Consider adding URL sanitization.The function should ensure the returned URL is safe and relative to prevent open redirect vulnerabilities.
Let's verify if there are any existing URL sanitization utilities in the codebase:
Consider adding URL sanitization to ensure the returned URL is safe:
const sanitizeUrl = (url: string): string => { // Remove protocol and domain, keep only path and query try { const urlObj = new URL(url); return `${urlObj.pathname}${urlObj.search}`; } catch { return url.replace(/^(?:https?:)?\/\/[^/]+/, ''); } };src/server/adminJs/tabs/projectsTab.ts (4)
58-58
: LGTM: Clean import of the new utility function.The import statement is well-placed and follows the project's import organization pattern.
192-192
: Well-structured implementation of the redirect utility.The
getRedirectUrl
utility is consistently implemented across multiple functions, replacing hardcoded redirect URLs. This improves maintainability and provides a centralized way to handle redirects.Also applies to: 259-259, 421-421, 531-531
Line range hint
347-374
: Correct handling of download redirect.The
exportEmails
function correctly maintains its specific download URL pattern (/admin/download/${path.basename(filePath)}
), which is appropriate for file downloads and should not be replaced with the general redirect utility.
Line range hint
605-632
: Verify comprehensive implementation of redirect utility.While the current implementation looks good, let's verify if there are any other functions in the file that handle redirects and should be updated to use the new utility.
✅ Verification successful
Let me try a different approach to find redirect patterns:
And let's specifically look at the project-related admin handlers:
Based on the
rg
results, I can see that inprojectsTab.ts
, there are multiple handlers that use redirects, and they are already properly using thegetRedirectUrl
utility function. The search shows that the following handlers in the file are using the utility:
- Multiple handlers using
const redirectUrl = getRedirectUrl(request, 'Project')
- Some error handlers with hardcoded paths that return to the main project resource page
The implementation is consistent across the file where dynamic redirects are needed, and error cases appropriately use hardcoded fallback URLs.
All relevant functions are already using the redirect utility appropriately
The implementation is comprehensive and consistent across the file. Error handlers appropriately use direct paths for fallback scenarios, while main handlers use the utility function for dynamic redirects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other potential redirect URL implementations that might need updating rg -l 'redirectUrl.*=.*["\']\/admin' .Length of output: 93
Script:
#!/bin/bash # Search for other potential redirect URL implementations that might need updating rg -l "redirectUrl.*=.*'/admin" . rg -l 'redirectUrl.*="/admin' .Length of output: 72
Script:
#!/bin/bash # Search for redirect-related patterns in the codebase rg -A 3 "redirectUrl.*[:=]" .Length of output: 10903
Script:
#!/bin/bash # Look for other handlers in the projects tab file ast-grep --pattern 'export const $_ = async ($_) => { $$$ return { redirectUrl: $_, $$$ } }'Length of output: 109
src/server/adminJs/adminJsUtils.ts
Outdated
// Default fallback URL if no referer is found | ||
const defaultUrl = `/admin/resources/${resourceName}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate resourceName parameter.
The resourceName
parameter should be validated to prevent potential security issues with path manipulation.
+const isValidResourceName = (name: string): boolean => {
+ return /^[a-zA-Z0-9-_]+$/.test(name);
+};
+
// Default fallback URL if no referer is found
+if (!isValidResourceName(resourceName)) {
+ throw new Error(`Invalid resource name: ${resourceName}`);
+}
const defaultUrl = `/admin/resources/${resourceName}`;
Committable suggestion was skipped due to low confidence.
src/server/adminJs/adminJsUtils.ts
Outdated
const refererIndex = request.rawHeaders.findIndex(h => h.toLowerCase() === 'referer'); | ||
const referrerUrl = refererIndex !== -1 ? request.rawHeaders[refererIndex + 1] : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve header extraction robustness.
The current header extraction method is fragile as it assumes headers are in a specific format. Consider using a more robust approach.
-const refererIndex = request.rawHeaders.findIndex(h => h.toLowerCase() === 'referer');
-const referrerUrl = refererIndex !== -1 ? request.rawHeaders[refererIndex + 1] : '';
+const getReferrerUrl = (headers: string[]): string => {
+ for (let i = 0; i < headers.length - 1; i += 2) {
+ if (headers[i].toLowerCase() === 'referer') {
+ return headers[i + 1];
+ }
+ }
+ return '';
+};
+const referrerUrl = getReferrerUrl(request.rawHeaders);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const refererIndex = request.rawHeaders.findIndex(h => h.toLowerCase() === 'referer'); | |
const referrerUrl = refererIndex !== -1 ? request.rawHeaders[refererIndex + 1] : ''; | |
const getReferrerUrl = (headers: string[]): string => { | |
for (let i = 0; i < headers.length - 1; i += 2) { | |
if (headers[i].toLowerCase() === 'referer') { | |
return headers[i + 1]; | |
} | |
} | |
return ''; | |
}; | |
const referrerUrl = getReferrerUrl(request.rawHeaders); |
01cd0fd
to
cdfd23e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/server/adminJs/adminJsUtils.ts (1)
1-6
: Enhance JSDoc documentation.Consider adding return type and error handling details to the documentation.
/** * Extracts the redirect URL from request headers for AdminJS actions * @param request - The AdminJS action request object * @param resourceName - The name of the resource (e.g., 'Project', 'User') - * @returns The URL to redirect to after action completion + * @returns {string} The pathname with search params from referrer URL, or default resource URL + * @throws {Error} If resourceName contains invalid characters */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/config.ts (0 hunks)
- src/server/adminJs/adminJsUtils.ts (1 hunks)
- src/server/adminJs/tabs/projectsTab.ts (9 hunks)
💤 Files with no reviewable changes (1)
- src/config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/adminJs/tabs/projectsTab.ts
🔇 Additional comments (1)
src/server/adminJs/adminJsUtils.ts (1)
7-7
: Previous type safety concern is still valid.The use of
any
type should be addressed as mentioned in the previous review.
src/server/adminJs/adminJsUtils.ts
Outdated
const refererIndex = request.rawHeaders.findIndex( | ||
h => h.toLowerCase() === 'referer', | ||
); | ||
const referrerUrl = | ||
refererIndex !== -1 ? request.rawHeaders[refererIndex + 1] : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add safety checks for header extraction.
The current implementation could throw runtime errors if rawHeaders
is undefined or if the array access is out of bounds.
+ if (!request?.rawHeaders?.length) {
+ return defaultUrl;
+ }
const refererIndex = request.rawHeaders.findIndex(
h => h.toLowerCase() === 'referer',
);
const referrerUrl =
- refererIndex !== -1 ? request.rawHeaders[refererIndex + 1] : '';
+ refererIndex !== -1 && refererIndex + 1 < request.rawHeaders.length
+ ? request.rawHeaders[refererIndex + 1]
+ : '';
Committable suggestion was skipped due to low confidence.
src/server/adminJs/adminJsUtils.ts
Outdated
try { | ||
if (referrerUrl) { | ||
const url = new URL(referrerUrl); | ||
return url.pathname + url.search; | ||
} | ||
} catch (error) { | ||
console.warn('Error parsing referrer URL:', error); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhance error handling and URL validation.
The error handling could be more informative and the URL validation more robust.
try {
if (referrerUrl) {
+ // Validate URL format
+ if (!referrerUrl.startsWith('http://') && !referrerUrl.startsWith('https://')) {
+ throw new Error('Invalid URL protocol');
+ }
const url = new URL(referrerUrl);
+ // Ensure URL is from the same origin
+ if (url.origin !== request.headers.origin) {
+ console.warn('Cross-origin redirect attempted');
+ return defaultUrl;
+ }
return url.pathname + url.search;
}
} catch (error) {
- console.warn('Error parsing referrer URL:', error);
+ console.warn('Error processing redirect URL:', {
+ error: error.message,
+ referrerUrl,
+ resourceName
+ });
}
Committable suggestion was skipped due to low confidence.
fc7ba65
to
75ac033
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/server/adminJs/tabs/projectsTab.ts (1)
Line range hint
421-438
: Consider adding error handling for redirectUrlWhile the dynamic URL generation is correctly implemented, consider adding error handling for cases where the redirect URL generation fails.
const redirectUrl = getRedirectUrl(request, 'Project'); +if (!redirectUrl) { + logger.error('Failed to generate redirect URL'); + return { + record: record.toJSON(currentAdmin), + notice: { + message: 'An error occurred while processing the request', + type: 'error', + }, + }; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/config.ts (0 hunks)
- src/server/adminJs/adminJs-types.ts (1 hunks)
- src/server/adminJs/adminJsUtils.ts (1 hunks)
- src/server/adminJs/tabs/projectsTab.ts (9 hunks)
💤 Files with no reviewable changes (1)
- src/config.ts
🧰 Additional context used
🪛 GitHub Check: test
src/server/adminJs/adminJsUtils.ts
[failure] 10-10:
Replace·request?.rawHeaders?.findIndex(⏎····h·=>·h.toLowerCase()·===·'referer',⏎··
with⏎····request?.rawHeaders?.findIndex(h·=>·h.toLowerCase()·===·'referer'
🔇 Additional comments (4)
src/server/adminJs/tabs/projectsTab.ts (4)
58-58
: LGTM: Import of redirect utilityThe import of
getRedirectUrl
aligns with the PR objective of implementing custom redirect functionality.
192-192
: LGTM: Dynamic redirect URL in verifyProjectsThe function now uses the new utility to generate redirect URLs dynamically, replacing hardcoded URLs while maintaining existing error handling.
Also applies to: 241-241
259-259
: LGTM: Consistent redirect handling in updateStatusOfProjectsThe function follows the same pattern of dynamic URL generation, maintaining consistency across the codebase.
Also applies to: 325-325
531-531
: LGTM: Robust redirect handling in listDelistThe function implements dynamic URL generation while maintaining robust error handling within its try-catch block.
Also applies to: 595-595
src/server/adminJs/adminJsUtils.ts
Outdated
const refererIndex = request?.rawHeaders?.findIndex( | ||
h => h.toLowerCase() === 'referer', | ||
) || -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the logical OR condition in findIndex.
The current implementation request?.rawHeaders?.findIndex(...) || -1
has a logical error. The || -1
will override the -1
returned by findIndex
when no match is found. This makes the fallback redundant and could cause unexpected behavior.
- const refererIndex = request?.rawHeaders?.findIndex(
- h => h.toLowerCase() === 'referer',
- ) || -1;
+ const refererIndex = request?.rawHeaders?.findIndex(
+ h => h.toLowerCase() === 'referer',
+ );
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const refererIndex = request?.rawHeaders?.findIndex( | |
h => h.toLowerCase() === 'referer', | |
) || -1; | |
const refererIndex = request?.rawHeaders?.findIndex( | |
h => h.toLowerCase() === 'referer', | |
); |
🧰 Tools
🪛 GitHub Check: test
[failure] 10-10:
Replace·request?.rawHeaders?.findIndex(⏎····h·=>·h.toLowerCase()·===·'referer',⏎··
with⏎····request?.rawHeaders?.findIndex(h·=>·h.toLowerCase()·===·'referer'
src/server/adminJs/adminJsUtils.ts
Outdated
try { | ||
if (referrerUrl) { | ||
const url = new URL(referrerUrl); | ||
return url.pathname + url.search; | ||
} | ||
} catch (error) { | ||
logger.error('Error parsing referrer URL:', error); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add origin validation to prevent open redirect vulnerability.
The current implementation accepts any URL from the referer header without validating its origin. This could potentially lead to open redirect vulnerabilities if an attacker manipulates the referer header.
try {
if (referrerUrl) {
const url = new URL(referrerUrl);
+ // Get allowed origins from configuration
+ const allowedOrigins = process.env.ALLOWED_ORIGINS?.split(',') || [];
+ // Validate URL origin
+ if (!allowedOrigins.includes(url.origin)) {
+ logger.warn('Invalid origin detected in redirect URL', {
+ origin: url.origin,
+ referrerUrl
+ });
+ return defaultUrl;
+ }
return url.pathname + url.search;
}
} catch (error) {
logger.error('Error parsing referrer URL:', error);
}
Committable suggestion was skipped due to low confidence.
75ac033
to
53a6d7f
Compare
53a6d7f
to
8a26b36
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thx @CarlosQ96
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @CarlosQ96 LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/server/adminJs/adminJsUtils.ts (2)
3-8
: Enhance JSDoc documentation with type information and return value details.
The documentation is well-structured but could be more specific about parameter types and return value format.
/**
* Extracts the redirect URL from request headers for AdminJS actions
- * @param request - The AdminJS action request object
- * @param resourceName - The name of the resource (e.g., 'Project', 'User')
- * @returns The URL to redirect to after action completion
+ * @param {AdminJsRequest} request - The AdminJS action request object containing rawHeaders
+ * @param {string} resourceName - The name of the resource (e.g., 'Project', 'User')
+ * @returns {string} A URL path with optional query parameters, prefixed with '/admin/resources/'
*/
27-29
: Enhance error logging with context information.
The current error logging lacks context that could be helpful for debugging issues in production.
- logger.error('Error parsing referrer URL:', error);
+ logger.error('Error parsing referrer URL:', {
+ error: error instanceof Error ? error.message : String(error),
+ resourceName,
+ referrerUrl,
+ stack: error instanceof Error ? error.stack : undefined,
+ });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/config.ts (0 hunks)
- src/server/adminJs/adminJs-types.ts (1 hunks)
- src/server/adminJs/adminJsUtils.ts (1 hunks)
- src/server/adminJs/tabs/projectsTab.ts (9 hunks)
💤 Files with no reviewable changes (1)
- src/config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/adminJs/adminJs-types.ts
- src/server/adminJs/tabs/projectsTab.ts
const refererIndex = | ||
request?.rawHeaders?.findIndex(h => h.toLowerCase() === 'referer') || -1; | ||
const referrerUrl = | ||
refererIndex !== -1 ? request.rawHeaders[refererIndex + 1] : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve referrer URL extraction logic.
The current implementation has potential issues with falsy values and type coercion.
- const referrerUrl =
- refererIndex !== -1 ? request.rawHeaders[refererIndex + 1] : false;
+ const referrerUrl =
+ refererIndex !== -1 && refererIndex + 1 < request.rawHeaders.length
+ ? request.rawHeaders[refererIndex + 1]
+ : null;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const refererIndex = | |
request?.rawHeaders?.findIndex(h => h.toLowerCase() === 'referer') || -1; | |
const referrerUrl = | |
refererIndex !== -1 ? request.rawHeaders[refererIndex + 1] : false; | |
const refererIndex = | |
request?.rawHeaders?.findIndex(h => h.toLowerCase() === 'referer') || -1; | |
const referrerUrl = | |
refererIndex !== -1 && refererIndex + 1 < request.rawHeaders.length | |
? request.rawHeaders[refererIndex + 1] | |
: null; |
if (url.pathname === `/admin/resources/${resourceName}` && !url.search) { | ||
return `${url.pathname}?timestamp=${Date.now()}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider using higher precision timestamp.
Using Date.now()
might not provide enough granularity for rapid successive requests, potentially leading to cache issues.
- return `${url.pathname}?timestamp=${Date.now()}`;
+ return `${url.pathname}?timestamp=${Date.now()}.${performance.now().toString().replace('.', '')}`;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if (url.pathname === `/admin/resources/${resourceName}` && !url.search) { | |
return `${url.pathname}?timestamp=${Date.now()}`; | |
} | |
if (url.pathname === `/admin/resources/${resourceName}` && !url.search) { | |
return `${url.pathname}?timestamp=${Date.now()}.${performance.now().toString().replace('.', '')}`; | |
} |
Issue: https://github.com/orgs/Giveth/projects/9/views/1?sliceBy%5Bvalue%5D=CarlosQ96&pane=issue&itemId=68913789&issue=Giveth%7Cimpact-graph%7C884
Fixes the project custom actions not reloading ui to reflect changes. Made a custom function in case some other entity needs it.
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Configuration Changes
XDAI_NODE_HTTP_URL
a required configuration field, refining environment variable management.Interface Updates
rawHeaders
property to the AdminJS request interface, allowing for more flexible data handling.